… dependency arrays (#1522)

Summary: Pull Request resolved: #1522

Differential Revision: D77254860

Pulled By: robhogan

… dependency arrays (#1522)

Summary:
In Metro, optional dependencies are dynamic imports or requires whose failure to resolve is not treated as a build time error. Currently `import()` or `require()` calls statically within a `try {}` are considered optional, when `transformer.allowOptionalDependencies` is enabled.

## The problem

A longstanding bug exists with optional dependencies due to the way they are filtered out if they cannot be resolved during graph traversal, so that the array of resolved dependencies is shorter than the array of collected dependencies.

| idx | collected dependencies | resolved dependencies |
|---|-------------| -- 
| 0 | './a.js'        | '/project/a.js'
| 1 | './optional.js' | '/project/c.js'
| 2 | './c.js'        |

This breaks the relationship between a module's `_dependencyMap` and the indices used in Metro `require` calls, assigned before resolution, eg:

`require('./optional.js')`

Is transformed to:

`_$$_REQUIRE(_dependencyMap[1], "./optional.js")`

Which will in fact load the module at `/project/c.js`, by the table above. Moreover, `require('./c.js')` will throw at runtime because `_dependencyMap[2]` is `undefined`.

Note that this behaves roughly as expected *only* if all unresolved dependencies are at the end of the array (later in the AST traversal of dependency collection).

## This change

Here, rather than filtering out unresolved dependencies, we retain them, propagating them to `null`s in the `_dependencyMap`, thus preserving ordering.

| idx | collected dependencies | resolved dependencies |
|---|-------------| -- 
| 0 | './a.js'        | '/project/a.js'
| 1 | './optional.js' | null
| 2 | './c.js'        | '/project/c.js'

We also explicitly handle them in `metroRequire` to throw a runtime "Cannot find module" error, consistent with Node.js.

## Changelog
```
 - **[Fix]** Fix `require` following missing optional dependencies returning wrong modules or failing to load
```


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

## Before
Because unresolvable dependencies are simply omitted from the `_dependencyMap` of the module, dependency indices are off-by-one, with bizarre results:

 {F1979783549}

## After

Module IDs appear at the expected positions in dependency maps, so dependencies indexed after an optional dependency are correct:
{F1979791272} 

`metroRequire` specifically handles nulls and throws a runtime error with a message consistent with Node.js

 {F1979791102}

Differential Revision: D77254860

Pulled By: robhogan

… dependency arrays (#1522)

Summary:
In Metro, optional dependencies are dynamic imports or requires whose failure to resolve is not treated as a build time error. Currently `import()` or `require()` calls statically within a `try {}` are considered optional, when `transformer.allowOptionalDependencies` is enabled.

## The problem

A longstanding bug exists with optional dependencies due to the way they are filtered out if they cannot be resolved during graph traversal, so that the array of resolved dependencies is shorter than the array of collected dependencies.

| idx | collected dependencies | resolved dependencies |
|---|-------------| -- 
| 0 | './a.js'        | '/project/a.js'
| 1 | './optional.js' | '/project/c.js'
| 2 | './c.js'        |

This breaks the relationship between a module's `_dependencyMap` and the indices used in Metro `require` calls, assigned before resolution, eg:

`require('./optional.js')`

Is transformed to:

`_$$_REQUIRE(_dependencyMap[1], "./optional.js")`

Which will in fact load the module at `/project/c.js`, by the table above. Moreover, `require('./c.js')` will throw at runtime because `_dependencyMap[2]` is `undefined`.

Note that this behaves roughly as expected *only* if all unresolved dependencies are at the end of the array (later in the AST traversal of dependency collection).

## This change

Here, rather than filtering out unresolved dependencies, we retain them, propagating them to `null`s in the `_dependencyMap`, thus preserving ordering.

| idx | collected dependencies | resolved dependencies |
|---|-------------| -- 
| 0 | './a.js'        | '/project/a.js'
| 1 | './optional.js' | null
| 2 | './c.js'        | '/project/c.js'

We also explicitly handle them in `metroRequire` to throw a runtime "Cannot find module" error, consistent with Node.js.

## Changelog
```
 - **[Fix]** Fix `require` following missing optional dependencies returning wrong modules or failing to load
```


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

## Before
Because unresolvable dependencies are simply omitted from the `_dependencyMap` of the module, dependency indices are off-by-one, with bizarre results:

 {F1979783549}

## After

Module IDs appear at the expected positions in dependency maps, so dependencies indexed after an optional dependency are correct:
{F1979791272} 

`metroRequire` specifically handles nulls and throws a runtime error with a message consistent with Node.js

 {F1979791102}

Differential Revision: D77254860

Pulled By: robhogan

… dependency arrays (#1522)

Summary:
In Metro, optional dependencies are dynamic imports or requires whose failure to resolve is not treated as a build time error. Currently `import()` or `require()` calls statically within a `try {}` are considered optional, when `transformer.allowOptionalDependencies` is enabled.

## The problem

A longstanding bug exists with optional dependencies due to the way they are filtered out if they cannot be resolved during graph traversal, so that the array of resolved dependencies is shorter than the array of collected dependencies.

| idx | collected dependencies | resolved dependencies |
|---|-------------| -- 
| 0 | './a.js'        | '/project/a.js'
| 1 | './optional.js' | '/project/c.js'
| 2 | './c.js'        |

This breaks the relationship between a module's `_dependencyMap` and the indices used in Metro `require` calls, assigned before resolution, eg:

`require('./optional.js')`

Is transformed to:

`_$$_REQUIRE(_dependencyMap[1], "./optional.js")`

Which will in fact load the module at `/project/c.js`, by the table above. Moreover, `require('./c.js')` will throw at runtime because `_dependencyMap[2]` is `undefined`.

Note that this behaves roughly as expected *only* if all unresolved dependencies are at the end of the array (later in the AST traversal of dependency collection).

## This change

Here, rather than filtering out unresolved dependencies, we retain them, propagating them to `null`s in the `_dependencyMap`, thus preserving ordering.

| idx | collected dependencies | resolved dependencies |
|---|-------------| -- 
| 0 | './a.js'        | '/project/a.js'
| 1 | './optional.js' | null
| 2 | './c.js'        | '/project/c.js'

We also explicitly handle them in `metroRequire` to throw a runtime "Cannot find module" error, consistent with Node.js.

## Changelog
```
 - **[Fix]** Fix `require` following missing optional dependencies returning wrong modules or failing to load
```


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

## Before
Because unresolvable dependencies are simply omitted from the `_dependencyMap` of the module, dependency indices are off-by-one, with bizarre results:

 {F1979783549}

## After

Module IDs appear at the expected positions in dependency maps, so dependencies indexed after an optional dependency are correct:
{F1979791272} 

`metroRequire` specifically handles nulls and throws a runtime error with a message consistent with Node.js

 {F1979791102}

Differential Revision: D77254860

Pulled By: robhogan

… dependency arrays (#1522)

Summary:
In Metro, optional dependencies are dynamic imports or requires whose failure to resolve is not treated as a build time error. Currently `import()` or `require()` calls statically within a `try {}` are considered optional, when `transformer.allowOptionalDependencies` is enabled.

## The problem

A longstanding bug exists with optional dependencies due to the way they are filtered out if they cannot be resolved during graph traversal, so that the array of resolved dependencies is shorter than the array of collected dependencies.

| idx | collected dependencies | resolved dependencies |
|---|-------------| -- 
| 0 | './a.js'        | '/project/a.js'
| 1 | './optional.js' | '/project/c.js'
| 2 | './c.js'        |

This breaks the relationship between a module's `_dependencyMap` and the indices used in Metro `require` calls, assigned before resolution, eg:

`require('./optional.js')`

Is transformed to:

`_$$_REQUIRE(_dependencyMap[1], "./optional.js")`

Which will in fact load the module at `/project/c.js`, by the table above. Moreover, `require('./c.js')` will throw at runtime because `_dependencyMap[2]` is `undefined`.

Note that this behaves roughly as expected *only* if all unresolved dependencies are at the end of the array (later in the AST traversal of dependency collection).

## This change

Here, rather than filtering out unresolved dependencies, we retain them, propagating them to `null`s in the `_dependencyMap`, thus preserving ordering.

| idx | collected dependencies | resolved dependencies |
|---|-------------| -- 
| 0 | './a.js'        | '/project/a.js'
| 1 | './optional.js' | null
| 2 | './c.js'        | '/project/c.js'

We also explicitly handle them in `metroRequire` to throw a runtime "Cannot find module" error, consistent with Node.js.

## Changelog
```
 - **[Fix]** Fix `require` following missing optional dependencies returning wrong modules or failing to load
```


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

## Before
Because unresolvable dependencies are simply omitted from the `_dependencyMap` of the module, dependency indices are off-by-one, with bizarre results:

 {F1979783549}

## After

Module IDs appear at the expected positions in dependency maps, so dependencies indexed after an optional dependency are correct:
{F1979791272} 

`metroRequire` specifically handles nulls and throws a runtime error with a message consistent with Node.js

 {F1979791102}

Differential Revision: D77254860

Pulled By: robhogan

… dependency arrays (#1522)

Summary:
In Metro, optional dependencies are dynamic imports or requires whose failure to resolve is not treated as a build time error. Currently `import()` or `require()` calls statically within a `try {}` are considered optional, when `transformer.allowOptionalDependencies` is enabled.

## The problem

A longstanding bug exists with optional dependencies due to the way they are filtered out if they cannot be resolved during graph traversal, so that the array of resolved dependencies is shorter than the array of collected dependencies.

| idx | collected dependencies | resolved dependencies |
|---|-------------| -- 
| 0 | './a.js'        | '/project/a.js'
| 1 | './optional.js' | '/project/c.js'
| 2 | './c.js'        |

This breaks the relationship between a module's `_dependencyMap` and the indices used in Metro `require` calls, assigned before resolution, eg:

`require('./optional.js')`

Is transformed to:

`_$$_REQUIRE(_dependencyMap[1], "./optional.js")`

Which will in fact load the module at `/project/c.js`, by the table above. Moreover, `require('./c.js')` will throw at runtime because `_dependencyMap[2]` is `undefined`.

Note that this behaves roughly as expected *only* if all unresolved dependencies are at the end of the array (later in the AST traversal of dependency collection).

## This change

Here, rather than filtering out unresolved dependencies, we retain them, propagating them to `null`s in the `_dependencyMap`, thus preserving ordering.

| idx | collected dependencies | resolved dependencies |
|---|-------------| -- 
| 0 | './a.js'        | '/project/a.js'
| 1 | './optional.js' | null
| 2 | './c.js'        | '/project/c.js'

We also explicitly handle them in `metroRequire` to throw a runtime "Cannot find module" error, consistent with Node.js.

## Changelog
```
 - **[Fix]** Fix `require` following missing optional dependencies returning wrong modules or failing to load
```


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

## Before
Because unresolvable dependencies are simply omitted from the `_dependencyMap` of the module, dependency indices are off-by-one, with bizarre results:

 {F1979783549}

## After

Module IDs appear at the expected positions in dependency maps, so dependencies indexed after an optional dependency are correct:
{F1979791272} 

`metroRequire` specifically handles nulls and throws a runtime error with a message consistent with Node.js

 {F1979791102}

Differential Revision: D77254860

Pulled By: robhogan

See: react/metro@5f8548c
See: react/metro#1522

See: react/metro@5f8548c
See: react/metro#1522

See: react/metro@5f8548c
See: react/metro#1522

See: react/metro@5f8548c
See: react/metro#1522

See: react/metro@5f8548c
See: react/metro#1522

See: react/metro@5f8548c
See: react/metro#1522

See: react/metro@5f8548c
See: react/metro#1522

Summary:

Flip the default value of the `transformer.allowOptionalDependencies` Metro config option from `false` to `true` in `metro-config`'s own defaults.

This allows for patterns like:

```js
let dep = null;
try {
  // If not installed, allowOptionalDependencies ===
  //   true => throw at runtime, handled by catch
  //   false => throw at build time
  dep = require('maybe-installed-dependency');
} catch {
  dep = require('./my-fallback');
}
```

Which comes up in popular packages, eg [`lru-cache`](https://github.com/isaacs/node-lru-cache/blob/16b3a916662ab449d496b7b4b4f04132565d1d28/src/diagnostics-channel-esm.mts#L24-L29).

Note that this is *already the default* in:
 - `react-native/metro-config`: https://github.com/react/react-native/blob/a632f9efe24bac8b3a113c78469948f55bde0f5d/packages/metro-config/src/index.flow.js#L86
 - And `expo/metro-config`: https://github.com/expo/expo/blob/ab042edb8228e3873bdbd8e04b7c61cd3e00372e/packages/%40expo/metro-config/src/ExpoMetroConfig.ts#L423

There were previously known problems with `allowOptionalDependencies` and the behaviour of subsequently declared dependencies, and they were also broken under HMR if their existence changed. These were both fixed a year ago in #1522.

The heuristic was also quite limited, for example a `try { require() }` was considered optional but an `import().catch()` was not. That was fixed in #1697.

I can't think of any good reason not to enable this and fix the misalignment with RN/Expo.

With this enabled, dependencies referenced inside `try/catch` blocks (and dynamic `import()`s with rejection handlers) are treated as optional by default: an unresolvable optional dependency no longer fails the build, matching the common expectation for guarded requires. Projects can still opt out by explicitly setting `allowOptionalDependencies: false`.

Since this change doesn't alter observable runtime behaviour (it allows bundles to build that would otherwise fail to build), I'm inclined to regard it as non-breaking.

```
 - **[Feature]**: `allowOptionalDependencies` defaults to `true` to align with RN and Expo
```

Reviewed By: huntie

Differential Revision: D111577052

Summary:

Flip the default value of the `transformer.allowOptionalDependencies` Metro config option from `false` to `true` in `metro-config`'s own defaults.

This allows for patterns like:

```js
let dep = null;
try {
  // If not installed, allowOptionalDependencies ===
  //   true => throw at runtime, handled by catch
  //   false => throw at build time
  dep = require('maybe-installed-dependency');
} catch {
  dep = require('./my-fallback');
}
```

Which comes up in popular packages, eg [`lru-cache`](https://github.com/isaacs/node-lru-cache/blob/16b3a916662ab449d496b7b4b4f04132565d1d28/src/diagnostics-channel-esm.mts#L24-L29).

Note that this is *already the default* in:
 - `react-native/metro-config`: https://github.com/react/react-native/blob/a632f9efe24bac8b3a113c78469948f55bde0f5d/packages/metro-config/src/index.flow.js#L86
 - And `expo/metro-config`: https://github.com/expo/expo/blob/ab042edb8228e3873bdbd8e04b7c61cd3e00372e/packages/%40expo/metro-config/src/ExpoMetroConfig.ts#L423

There were previously known problems with `allowOptionalDependencies` and the behaviour of subsequently declared dependencies, and they were also broken under HMR if their existence changed. These were both fixed a year ago in #1522.

The heuristic was also quite limited, for example a `try { require() }` was considered optional but an `import().catch()` was not. That was fixed in #1697.

I can't think of any good reason not to enable this and fix the misalignment with RN/Expo.

With this enabled, dependencies referenced inside `try/catch` blocks (and dynamic `import()`s with rejection handlers) are treated as optional by default: an unresolvable optional dependency no longer fails the build, matching the common expectation for guarded requires. Projects can still opt out by explicitly setting `allowOptionalDependencies: false`.

Since this change doesn't alter observable runtime behaviour (it allows bundles to build that would otherwise fail to build), I'm inclined to regard it as non-breaking.

```
 - **[Feature]**: `allowOptionalDependencies` defaults to `true` to align with RN and Expo
```

Reviewed By: huntie

Differential Revision: D111577052

Summary:
Pull Request resolved: #1775

Flip the default value of the `transformer.allowOptionalDependencies` Metro config option from `false` to `true` in `metro-config`'s own defaults.

This allows for patterns like:

```js
let dep = null;
try {
  // If not installed, allowOptionalDependencies ===
  //   true => throw at runtime, handled by catch
  //   false => throw at build time
  dep = require('maybe-installed-dependency');
} catch {
  dep = require('./my-fallback');
}
```

Which comes up in popular packages, eg [`lru-cache`](https://github.com/isaacs/node-lru-cache/blob/16b3a916662ab449d496b7b4b4f04132565d1d28/src/diagnostics-channel-esm.mts#L24-L29).

Note that this is *already the default* in:
 - `react-native/metro-config`: https://github.com/react/react-native/blob/a632f9efe24bac8b3a113c78469948f55bde0f5d/packages/metro-config/src/index.flow.js#L86
 - And `expo/metro-config`: https://github.com/expo/expo/blob/ab042edb8228e3873bdbd8e04b7c61cd3e00372e/packages/%40expo/metro-config/src/ExpoMetroConfig.ts#L423

There were previously known problems with `allowOptionalDependencies` and the behaviour of subsequently declared dependencies, and they were also broken under HMR if their existence changed. These were both fixed a year ago in #1522.

The heuristic was also quite limited, for example a `try { require() }` was considered optional but an `import().catch()` was not. That was fixed in #1697.

I can't think of any good reason not to enable this and fix the misalignment with RN/Expo.

With this enabled, dependencies referenced inside `try/catch` blocks (and dynamic `import()`s with rejection handlers) are treated as optional by default: an unresolvable optional dependency no longer fails the build, matching the common expectation for guarded requires. Projects can still opt out by explicitly setting `allowOptionalDependencies: false`.

Since this change doesn't alter observable runtime behaviour (it allows bundles to build that would otherwise fail to build), I'm inclined to regard it as non-breaking.

```
 - **[Feature]**: `allowOptionalDependencies` defaults to `true` to align with RN and Expo
```

Reviewed By: huntie

Differential Revision: D111577052

fbshipit-source-id: a85094df7f2d06387715160510c62462387f4c96